Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DWB] Option to limit velocity commands in trajectory generator #4663

Merged
merged 6 commits into from
Sep 12, 2024

Conversation

huiyulhy
Copy link
Contributor

@huiyulhy huiyulhy commented Sep 8, 2024


Basic Info

Info Please fill out this column
Ticket(s) this addresses Issue #3277
Primary OS tested on Ubuntu 22.04
Robotic platform tested on Turtlebot3 Gazebo simulation - waffle
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • Added an option in the standard trajectory generator in DWB to allow the user to limit the velocity command in the trajectory
    -based on current trajectory through a new parameter (limit_vel_cmd_in_traj_)
  • This option ensures that the acceleration constraints are respected in the event there is a large difference between the current and commanded velocity - which may be more useful when there is no velocity smoother in use
  • limit_vel_cmd_in_traj_ is set to false by default, but when set to true, would use the first velocity computed in the trajectory rollout as the trajectory velocity

Default behavior:
dev_default

When limit_vel_cmd_in_traj_ is set to true:
proposed_default

We observed slightly improved velocity tracking, and similar time to reach the same goal setting this parameter to true.

Description of documentation updates required from your changes

  • Added parameter limit_vel_cmd_in_traj_ to DWB local planner (in standard trajectory generator)

Future work that may be required in bullet points

  • This parameter could also be extensible to other trajectory generation methods within DWB

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! We also need:

@huiyulhy
Copy link
Contributor Author

huiyulhy commented Sep 11, 2024

Great! We also need:

Thanks for the review and pointing the required changes out! Am updating for the documentation for Nav2 and I wanted to confirm that the addition of this change in the migration guide would be from Jazzy to K-Turtle?

@huiyulhy huiyulhy marked this pull request as draft September 11, 2024 15:49
@SteveMacenski
Copy link
Member

That is correct!

Signed-off-by: huiyulhy <[email protected]>
@huiyulhy
Copy link
Contributor Author

Great! We also need:

@SteveMacenski thanks for the feedback! Fixed the linting errors and updated the documentation in this PR on nav2 docs!

@huiyulhy huiyulhy marked this pull request as ready for review September 12, 2024 06:52
Signed-off-by: huiyulhy <[email protected]>
Signed-off-by: huiyulhy <[email protected]>
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...roller/dwb_plugins/src/standard_traj_generator.cpp 80.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
...ns/include/dwb_plugins/standard_traj_generator.hpp 100.00% <ø> (ø)
...roller/dwb_plugins/src/standard_traj_generator.cpp 97.67% <80.00%> (-1.10%) ⬇️

... and 4 files with indirect coverage changes

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of docs updates needed, then we can merge the pair!

@SteveMacenski SteveMacenski merged commit 271d2d6 into ros-navigation:main Sep 12, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants